Skip to content

feat: add minimum length requirement to propagator configuration#173

Closed
codeboten wants to merge 1 commit intoopen-telemetry:mainfrom
codeboten:codeboten/nonemptystring
Closed

feat: add minimum length requirement to propagator configuration#173
codeboten wants to merge 1 commit intoopen-telemetry:mainfrom
codeboten:codeboten/nonemptystring

Conversation

@codeboten
Copy link
Copy Markdown
Contributor

@codeboten codeboten commented Feb 18, 2025

BREAKING CHANGE: This is potentially breaking any implementation that ignores a blank value for a propagator today.

Fixes #172

@codeboten
feat: add minimum length requirement to propagator configuration
54a93ea 
BREAKING CHANGE: This is potentially breaking any implementation that ignores a blank value for a propagator today.

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten codeboten requested a review from a team as a code owner February 18, 2025 18:19
@marcalff
Copy link
Copy Markdown
Member

marcalff commented Feb 18, 2025

Maybe the issue is that propagators should not be an array of strings in the first place.

Instead of:

propagator:
  composite: [ tracecontext, baggage, b3, b3multi, jaeger, xray, ottrace ]

Can we have:

propagator:
  - tracecontext
  - baggage
  - b3
  - b3multi
  - jaeger
  - xray
  - ottrace

For every extension point (exporters, processors, etc), the name appears as a yaml node.

For propagators, the name appears as data, this does not follows the general pattern.

Custom propagators are extension points in my understanding, because they will need to be registered in the SDK, they are not just "strings".

@jack-berg
Copy link
Copy Markdown
Member

jack-berg commented Mar 3, 2025

@marcalff I think you mean:

propagator:
  - tracecontext:
  - baggage:
  - b3:
  - b3multi:
  - jaeger:
  - xray:
  - ottrace:

@jack-berg
Copy link
Copy Markdown
Member

jack-berg commented Mar 3, 2025

For propagators, the name appears as data, this does not follows the general pattern.

I agree we should fix this.

Should also probably add backwards compatibility with OTEL_PROPAGATORS following our data modeling and environment variable substitution rule.

End config might look like:

propagator:
  composite:
      - tracecontext:
      - baggage:
      - b3:
      - b3multi:
      - jaeger:
      - xray:
      - ottrace:
  # The value is a list of comma separated key-value pairs matching the format of OTEL_PROPAGATORS. See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#general-sdk-configuration for details.
  # Merged with .propagator.composite. If an entry appears in both, .propagator.composite takes priority.
  composite_list: ${OTEL_PROPAGATORS}

@jack-berg jack-berg mentioned this pull request Mar 6, 2025
Merged
marcalff
marcalff requested changes Mar 10, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking this fix in favor of PR #187
Having dedicated yaml nodes for extension points is more aligned with the existing codebase.

@codeboten
Copy link
Copy Markdown
Contributor Author

codeboten commented Mar 11, 2025

Closing as addressed in #187

@codeboten codeboten closed this Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcalff marcalff marcalff requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

propagator name should not be a blank string

3 participants

@codeboten @marcalff @jack-berg